Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

always use programUrl (instead of programName) #300

Closed
wants to merge 1 commit into from

Conversation

mediaminister
Copy link
Collaborator

@mediaminister mediaminister commented Jun 9, 2019

This pull request includes:

  • statichelper functions to convert program, short programUrl, long programUrl and targetUrl between each other:
    Examples:
    program: de-campus-cup
    short programUrl: /vrtnu/a-z/de-campus-cup/
    long programUrl: //www.vrt.be/vrtnu/a-z/de-campus-cup/
    targetUrl: //www.vrt.be/vrtnu/a-z/de-campus-cup.relevant/
  • application of these functions in favorites and vrtapihelper classes
  • clearer variable names
  • fix for an unnoticed "Universiteit van Vlaanderen" seasons menu bug
  • all necessary unit test fixes
  • cleaner plugin://plugin.video.vrt.nu/ url parameters

@dagwieers
Copy link
Collaborator

There are a few things I don't like:

  • The interface for users becomes more complicated then it should be, we should be passing the shortest form IMO. So de-campus-cup and everything could be translated to what the API needs instead
  • I would call this short name 'program' so that every interface simply requires: &program=<program>. We don't need to teach users the differences between short programUrl, long programUrl, programName or targetUrl
  • We eventually may want to make the interface cleaner (now we reuse video_url for various things IIRC)

@mediaminister
Copy link
Collaborator Author

The interface for users becomes more complicated then it should be

With "interface", you mean the plugin://plugin.video.vrt.nu/ url? Okay then, I'll look into this to simplify the parameters.

@dagwieers
Copy link
Collaborator

dagwieers commented Jun 10, 2019

Correct, this is mostly used for autostart or automation, where users are exposed to our interface. That's also why I don't like the current ?action=&video_url= and would prefer the router module and a nice syntax instead.

But we can't change it for the existing playable items, as that would make everything already watched unwatched again. So any change to that should be delayed for a v2 release. However for favorites and some of the other actions with a very low probability that they are automated/stored as favorites we have a chance of improving the interface.

@dagwieers
Copy link
Collaborator

dagwieers commented Jun 10, 2019

I really like your new implementation, it simplifies quite a few parts of the code.

I have two remarks and one concern though:

  1. the very long function names (but this is not a showstopper, long names are clearer)
  2. we probably want unit tests for the new functions to ensure they keep working as expected
  3. this will break Kodi favorites that users saved (so this would likely be v2 material)

But maybe we should have this discussion now anyway and make the next release v2 ?
(And in that case we may also want to improve the playable items ?)

@dagwieers dagwieers added enhancement New feature or request bug Something isn't working labels Jun 10, 2019
@mediaminister mediaminister force-pushed the programUrl branch 3 times, most recently from 7b2aa5a to e82a84a Compare June 11, 2019 09:54
@mediaminister
Copy link
Collaborator Author

mediaminister commented Jun 11, 2019

  1. I combined the different statichelper functions, so it's shorter and easier to use: url_to_program(url) and program_to_url(program, url_type)
  2. I added a unit test for get_latest_episode and extended the favorites follow/unfollow test with multiple programs. I think we don't need separate unit tests for these new statichelper functions because these functions are used in vrtapihelper and favorites classes that also have unit tests.
  3. Yes this breaks Kodi favorites, but you asked to change the interface.

I think it's a good idea to change the target branch of this pull request and merge this to a v2.0 branch.

About the plugin://plugin.video.vrt.nu/?action=play interface:
In the current version we pass three parameters: video_id, publication_id and video_url to the streamservice class. For VOD, the api needs video_id, publication_id, for livestreams the api needs only a video_id. The parameter video_url is only used for web scraping.
Changing these parameters will break a lot of the current code. I didn't found a solution yet to use a nice and clean syntax for this "play" interface. This needs more research and will definitely not be part of this pull request.

@dagwieers
Copy link
Collaborator

dagwieers commented Jun 11, 2019

  1. Yes this breaks Kodi favorites, but you asked to change the interface.

What you implement is close to my ideal implementation, but because it breaks existing bookmarks/automation I was happy with only the required changes for favorites (and some other interfaces). But now that we have this, maybe we should just bite the bullet and agree on the holistic approach for v2.

PS If I would make a v2. branch, we may still need to close this PR and open a new PR against the v2 branch. I would rather not have 2 main branches open for a long period of time though. So I wonder if we even have to do a v1.10.1 release ? (and in case we do, we can easily open a new 1.10 branch for fixing whatever comes our way and keep master to upcoming v2 branch ?)
PS2 I was wondering if it would be acceptable to rewrite the stored information from our plugin in the Kodi (sqlite or other) database so that existing URI's are rewritten according to the new structure and things watched would simple keep on working.

@mediaminister
Copy link
Collaborator Author

mediaminister commented Jun 11, 2019

I don't know when we will have a good working v2 version, so I think a v1.10.1 branch with bug fixes is needed. We can keep the master branch for v2 development
I will push a new v1.10.1 branch with minor changes which fixes this bug and the "Universiteit van Vlaanderen" seasons menu bug: https://github.com/pietje666/plugin.video.vrt.nu/tree/v1.10.1

Editing the database is possible and acceptable but unnecessary work.

@mediaminister
Copy link
Collaborator Author

Closing this, because this mixes up v1.10.1 and v2 functionality.

@mediaminister mediaminister deleted the programUrl branch June 14, 2019 14:20
@dagwieers dagwieers added this to the v2.0.0 milestone Jun 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants